-
Notifications
You must be signed in to change notification settings - Fork 142
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix Page Designer ImageWithText Link component #1092
Conversation
packages/template-retail-react-app/app/page-designer/assets/utils.js
Outdated
Show resolved
Hide resolved
import {isAbsoluteURL} from '../utils' | ||
|
||
const LinkWrapper = ({ITCLink, children}) => { | ||
const chakraLinkWrapper = (children) => <ChakraLink href={ITCLink}>{children}</ChakraLink> | ||
const linkWrapper = (children) => <Link to={ITCLink}>{children}</Link> | ||
|
||
return isAbsoluteURL(ITCLink) ? chakraLinkWrapper(children) : linkWrapper(children) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you look at the Link component from the template components folder you'll see that that component has similar logic built into it that you are duplicating here maybe? 🤔
I think there is a prop called useNavLink that you might be able to take advantage of.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think that is the case.
Reading our custom link component uses a react-router Link or NavLink component (used for the Breadcrumbs menu) with the Chakra Link styles.
<ChakraLink | |
as={useNavLink ? NavSPALink : SPALink} |
There's no logic to assert if the url
string is absolute, and if so, use a Chakra UI Link component instead of react-router component.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The prop useNavLink
only determines if we use react-router's Link vs NavLink component.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've resolved a similar issue recently by using the as
prop. Since the ...props
spread is after the as={useNavLink ? ...}
prop (L21 in the snippet below), you can override it by passing a custom as
to the Link
component.
const Link = React.forwardRef(({href, to, useNavLink = false, ...props}, ref) => { | |
const _href = to || href | |
const {buildUrl} = useMultiSite() | |
const updatedHref = buildUrl(_href) | |
return ( | |
<ChakraLink | |
as={useNavLink ? NavSPALink : SPALink} | |
{...(useNavLink && {exact: true})} | |
{...props} | |
to={updatedHref} | |
ref={ref} | |
/> | |
) | |
}) |
Meaning you could possibly adjust your current logic to something like:
import Link from '../path/to/Link'
const linkProps = ({ITCLink}) => {
return isAbsoluteURL(ITCLink) ? { as: ChakraLink } : {}
}
() => <Link {...linkProps(/* args */ )} >...</Link>
Additionally, I added a LinkComponent?: ComponentWithAs<'a'>
prop to our <Link/>
, which has a default value of <ChakraLink />
. This allows us to override the UI of the link (e.g. using Chakra UI's <LinkOverlay />
component) while retaining the react-router link logic from the as
prop.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @breadadams for the feedback. I played with the idea and you're right, we can override our custom react-router <Link />
component logic with the ChakraLink
logic by using as={ChakraLink}
. The declared as
prop passed to the <Link />
component will override the as
prop with ...props
spread.
But then we need to modify the <Link />
to conditionally use href
or to
props depending on if we use ChakraLink
or a react-router-like Link
component.
The LinkComponent
idea to override styles and keep the react-router Link logic makes sense as well, but today we use the layerStyle
prop pattern to override styles.
The scope of the PR is to fix the Link in the ImageWithText
Page designer component. We plan to extract the Page designer code from the template in the future. In this context, I think it's better to solve the problem in the ImageWithText
component file. But both ideas are good suggestions to make our custom <Link />
component more flexible in future extensibility work.
packages/template-retail-react-app/app/page-designer/assets/image-with-text/index.jsx
Outdated
Show resolved
Hide resolved
…age-with-text/index.jsx Co-authored-by: Ben Chypak <[email protected]>
* Fix PD ImageWithText Link component by using Chakra UI Link * Use isAbsoluteURL to use react-router Link or Chakra Link component * PR Feedback * Clean up * Update packages/template-retail-react-app/app/page-designer/assets/image-with-text/index.jsx * Remove temporal page-viewer page to facilitate review --------- Co-authored-by: Ben Chypak <[email protected]>
* v3: [Spike]Replace watch with nodemon (#1146) Fix Page Designer ImageWithText Link component (#1092) (#1148) Upgrade deprecated dependencies (#1124) Restart login flow if refresh_token is invalid (#1135) Move some util to site-utils to avoid circular imports (#1133) Restore old file name. (#1140) Update eslint configuration (#1129) CI: clarify the environment variables (#1127) Remove react-query-devtools from production build (#1121) Update lerna.json (#1118) Parallelize lighthouse ci (#1126) Increase test timeouts only on CI env (#1123) Remove unused `request` deprecated dependency. (#1125) Upgrade msw to latest (#1100) Add mergeBasket hook (#1114) [V3][Hooks Integration 🪝] Manually update cache for ShopperCustomer (#1113) dont use callback on mutateAsync (#1119) 2-spaces not 4-spaces (#1117) # Conflicts: # packages/internal-lib-build/package-lock.json # packages/pwa-kit-create-app/package-lock.json # packages/pwa-kit-dev/package-lock.json # packages/pwa-kit-dev/package.json # packages/pwa-kit-dev/src/configs/webpack/config.js # packages/pwa-kit-dev/src/ssr/server/build-dev-server.js # packages/pwa-kit-react-sdk/package-lock.json # packages/pwa-kit-runtime/package-lock.json # packages/template-express-minimal/package-lock.json # packages/template-mrt-reference-app/package-lock.json # packages/template-retail-react-app/package-lock.json # packages/template-typescript-minimal/package-lock.json # packages/test-commerce-sdk-react/package-lock.json
…-rehaul * feature/template-extensibility: [Spike]Replace watch with nodemon (#1146) Fix Page Designer ImageWithText Link component (#1092) (#1148) Upgrade deprecated dependencies (#1124) Restart login flow if refresh_token is invalid (#1135) Move some util to site-utils to avoid circular imports (#1133) Restore old file name. (#1140) Update eslint configuration (#1129) CI: clarify the environment variables (#1127) Remove react-query-devtools from production build (#1121) Update lerna.json (#1118) Parallelize lighthouse ci (#1126) Increase test timeouts only on CI env (#1123) Remove unused `request` deprecated dependency. (#1125) Upgrade msw to latest (#1100) Add mergeBasket hook (#1114) [V3][Hooks Integration 🪝] Manually update cache for ShopperCustomer (#1113) dont use callback on mutateAsync (#1119) 2-spaces not 4-spaces (#1117) # Conflicts: # packages/internal-lib-build/package-lock.json # packages/pwa-kit-dev/package-lock.json # packages/pwa-kit-react-sdk/package-lock.json # packages/pwa-kit-runtime/package-lock.json # packages/pwa-kit-runtime/package.json
…xtensibility-algo-refactor * feature/template-extensibility: split ssr build on local (#1155) (#1156) [V3] Update Page Designer (#1128) bring back deleted package lockfiles fix bad merge conflict [V3][Hooks Integration 🪝] Replace last of the `getProps` (#1149) Fix Page Designer ImageWithText Link component (#1092) # Conflicts: # packages/internal-lib-build/package-lock.json # packages/pwa-kit-create-app/package-lock.json # packages/template-express-minimal/package-lock.json # packages/template-mrt-reference-app/package-lock.json # packages/template-retail-react-app/app/components/_app-config/index.jsx # packages/template-retail-react-app/app/components/_app/index.jsx # packages/template-retail-react-app/app/page-designer/core/component/index.jsx # packages/template-retail-react-app/app/page-designer/core/component/index.test.js # packages/template-retail-react-app/app/page-designer/core/page/index.jsx # packages/template-retail-react-app/app/page-designer/core/page/index.test.js # packages/template-retail-react-app/app/page-designer/core/region/index.jsx # packages/template-retail-react-app/app/page-designer/core/region/index.test.js # packages/template-retail-react-app/app/page-designer/layouts/carousel/index.jsx # packages/template-retail-react-app/app/page-designer/layouts/mobileGrid1r1c/index.jsx # packages/template-retail-react-app/app/page-designer/layouts/mobileGrid2r1c/index.jsx # packages/template-retail-react-app/app/page-designer/layouts/mobileGrid2r2c/index.jsx # packages/template-retail-react-app/app/page-designer/layouts/mobileGrid2r3c/index.jsx # packages/template-retail-react-app/app/page-designer/layouts/mobileGrid3r1c/index.jsx # packages/template-retail-react-app/app/page-designer/layouts/mobileGrid3r2c/index.jsx # packages/template-retail-react-app/app/pages/page-not-found/index.jsx # packages/template-retail-react-app/app/utils/test-utils.js # packages/template-typescript-minimal/package-lock.json # packages/test-commerce-sdk-react/package-lock.json
* Update `develop` with `release-v2.7.0` (#1033) * Starting release process for 2.7.0 * Update Create MRT credentials file (#1025) * Release 2.7: fix order of hooks (#1027) * Return early only after all of the hooks are called * Bumper version --------- Co-authored-by: Ben Chypak <[email protected]> * Quick fix for einstein data (#1028) * Quick fix for einstein data * Bump Version * Re-lock package-lock files with npm 8 * Update package-lock.json * Update einstein.js * Regenerate lock files (#1030) * Regenerate lock files * Bump version to 2.7.0-alpha.3 * Bump to 2.7.0 (#1032) * Begin development on 2.8.0 --------- Co-authored-by: Adam Raya <[email protected]> Co-authored-by: Vincent Marta <[email protected]> * Move the MRT reference app to the SDKs, so that we can verify eg. Node support (#966) * BUG: Changed type of the phone number field to bring up numberic keyboard on mobile devices - W-9871940 (#1016) Co-authored-by: Ben Chypak <[email protected]> * Update Retail React App Page Designer integration README (#1041) Co-authored-by: Richard Sexton <[email protected]> * Implement `updateCustomerPassword` as no-op. (#1031) * Allow query hook parameters to be `null`. (#1046) * Remove unused util. * Allow query hook parameters to be `null`. * Fix addresses not having preferred address first. (#1051) * Fix addresses not having preferred address first. * Include all addresses, not just preferred address twice. * Correctly include preferred address. * Make `mergeBasket` Conditional More Robust (#1048) * Update merge logic * Update CHANGELOG.md * Lint * PR feedback * Rename isNewlyRegisters to isNew for simplicity * Lint * [commerce-sdk-react] Decode pre-fetched token and save auth data in storage (#1052) * add test * decode jwt data * lint * Update packages/commerce-sdk-react/src/auth/index.ts Co-authored-by: Will Harney <[email protected]> * rename parseSlasJWT * Update packages/commerce-sdk-react/src/auth/index.test.ts Co-authored-by: Will Harney <[email protected]> * convert all .thens to await * make fake token in tests more clear * lint --------- Co-authored-by: Will Harney <[email protected]> * Prevent modal to open when it fails to add an item to cart (#1053) * prevent modal to open when it fails to add an item to cart * Update comment --------- Co-authored-by: Alex Vuong <[email protected]> * Fix `getConfig` referencing config from incorrect location (#1049) * Initial Commit * Update CHANGELOG.md * Revert some testing code * Get test coverage back up * Test build directory folder before proceeding. * fix(template-retail-react-app): product-list refinements (#957) * fix(template-retail-react-app): product-list refinements + Ensure that the `selectedFilters` provided to the `Refinements` value components is always an array. And update said value components (checkbox, color, radio & size) to handle always receiving an array. + Apply some loose equality checks, catering for cases where a refinement URL param is parsed as a number but the refinement `.value` is a stringified number. + Fix bug where non-multiple filters could not be unchecked by selecting themselves (e.g. clicking an already checked checkbox). + Fix bug where a refinement URL param is parsed as a number, but was handled as a string (with `.split()`). + Fix bug where radio refinements would still appear as checked after clearing the value via `SelectedRefinements`. + Deprecate unused proptypes in `SizeRefinements` * feat(retail-react-app): update `CheckboxRefinements` + Adjust the `onChange` logic so that multiple checkbox refinements can be selected --------- Co-authored-by: Ben Chypak <[email protected]> * update docs for shopper-experience scope (#1059) * add docs for experience scope * Add `merge_group` event to trigger GA builds --------- Co-authored-by: Will Harney <[email protected]> Co-authored-by: Adam Raya Navarro <[email protected]> * Update lockfiles to reflect current version. (#1071) * [commerce-sdk-react] Implement remaining Shopper Baskets cache logic (#1070) * Refactor Shopper Basket cache following new pattern * Fix types * Update packages/commerce-sdk-react/src/hooks/ShopperBaskets/cache.ts Co-authored-by: Will Harney <[email protected]> * PR Feedback * Clean up unused utils and types after refactor * Add missing response basketId to queryKey * Implement cache for the remaining mutations * Tests WIP * PR Feedback & Use query key path in `deleteBasket` cache * Add tests for mutations returning void response * PR Feedback * Remove unused `and` hooks util * Revert "Remove unused `and` hooks util" This reverts commit c0a364a. --------- Co-authored-by: Will Harney <[email protected]> * [commerce-sdk-react] Fix Shopper Baskets Test case (#1082) * Fix Shopper Basket empty response test cases * lint * feat(pwa-kit-dev): minor performance improvements and added comments (#974) * feat(pwa-kit-dev): minor performance improvements and added comments * docs(pwa-kit-dev): clean up comments * refactor(pwa-kit-dev): update condition from PR feedback --------- Co-authored-by: Will Harney <[email protected]> * Update dependencies. (#1079) * Remove internal packages to bypass lerna nonsense. Include output.json to make restoring deps easier. * Disable build scripts to make changing deps easier. * Update root deps. * Fix typescript issue. * Create restore script to restore internal deps/scripts. * Update commerce-sdk-react deps. * Update test-commerce-sdk-react deps. * Update template-typescript-minimal deps * Improve restore script. * Fix trailing comma. * Update template-retail-react-app deps * npm prune everything * Update pwa-kit-runtime deps * Update pwa-kit-react-sdk deps * Update pwa-kit-create-app deps * Update pwa-kit-dev deps (except eslint) * Update pwa-kit-dev eslint deps * Update internal-lib-build deps (except eslint) * Update pwa-kit eslint deps * Restore internal deps. * Restore build scripts. * Remove temporary helper files. * Bump ua-parser-js to avoid vulnerable version. * Anchor cross-env common dep to version used by internal-lib-build. * Re-enable eslint. * Implement Cache Logic for Shopper APIs (Contexts/Customers/Login/Orders) (#1073) * Initial commit * Update packages/commerce-sdk-react/src/hooks/useAuthHelper.ts Co-authored-by: Will Harney <[email protected]> * Update packages/commerce-sdk-react/src/hooks/ShopperOrders/cache.ts Co-authored-by: Will Harney <[email protected]> * Update packages/commerce-sdk-react/src/hooks/ShopperOrders/cache.ts Co-authored-by: Will Harney <[email protected]> * Added root to all query keys, use remove over clear * Remove previous impemented "clear" from utils * Initial tests for shoppercontexts * Update ShopperLogin tests * Fix order tests * Update packages/commerce-sdk-react/src/hooks/ShopperContexts/cache.ts Co-authored-by: Will Harney <[email protected]> * Update cache.ts * Lint! * Update Json.tsx * Lint! * Testing race condition in tests * Re-add tests in other order. * Update CHANGELOG.md * Add todo to complete context cache work * Update packages/commerce-sdk-react/src/hooks/ShopperBaskets/mutation.test.ts Co-authored-by: Will Harney <[email protected]> * Update packages/commerce-sdk-react/src/components/ShopperExperience/Page/index.tsx Co-authored-by: Will Harney <[email protected]> * Update useAuthHelper.ts * Update packages/commerce-sdk-react/src/hooks/ShopperCustomers/cache.ts Co-authored-by: Adam Raya <[email protected]> --------- Co-authored-by: Will Harney <[email protected]> Co-authored-by: Adam Raya <[email protected]> * remove site alias and locale from location.state.directedFrom path (#1065) * remove site alias and locale from location.state.directedFrom path * moving call to removeSiteLocaleFromPath into use-navigation hook * fixing failing test, added tests for removeSiteLocaleFromPath * per code review, skipping failed test instead of using mocking * Fix Page Designer ImageWithText Link component (#1092) * Fix PD ImageWithText Link component by using Chakra UI Link * Use isAbsoluteURL to use react-router Link or Chakra Link component * PR Feedback * Clean up * Update packages/template-retail-react-app/app/page-designer/assets/image-with-text/index.jsx Co-authored-by: Ben Chypak <[email protected]> * Remove temporal page-viewer page to facilitate review --------- Co-authored-by: Ben Chypak <[email protected]> * split ssr build on local (#1155) * add suffix to ssr build files (#1157) * Added session bridge call to login for phased launch (#1159) * Added session bridge call to login for phased launch * Fix code smell for session-bridge in hybrid * Fix multi-value query params being lost (#1150) * Fix multi-value query params being lost * Update CHANGELOG.md * Snyk dependency updates (#1169) * Dependency updates * Update runtime package lock * Bump cosmiconfig version to latest * [Hybrid] PWA Kit should have a mechanism for replacing the access token when a SFRA login state is changed (#1171) * Implement mechanism to store refresh token copy and compare with sfra * Update tests and mocks for util function to check SFRA login state * Fix linting issues * FIx param types for util functionn * Rename old isTokenValid to isTokenExpired * Remove expiry for refresh_token in localstorage * Update packages/template-retail-react-app/app/commerce-api/utils.js Co-authored-by: Kevin He <[email protected]> * fix test * Fix linting on use-auth-modal.test.js * Update hasSFRAStateChanged logic to compare keys and values * Fix linting --------- Co-authored-by: Kevin He <[email protected]> * Add a redirect to login page after user signs out from checkout page (#1172) * Add a redirect to login page after user signs out from checkout page * Update CHANGELOG.md * Remove history since navigate handles similarly * Bump version number to 2.7.1-alpha.0 * Update changelogs * #1174 Replace invalid value for wrap property (#1179) * Update changelogs * Version bump to 2.7.1-preview.0 * Revert "Version bump to 2.7.1-preview.0" This reverts commit 985a7e0. * Update CHANGELOG.md * Rebuild lock files and fix ShopperLogin mutation test * Revert "Rebuild lock files and fix ShopperLogin mutation test" This reverts commit d9cfe50. * Add additional properties to ShopperLogin test types (#1185) * [V2] Re-generate lock files and fix hook lib tests (#1186) * re-generate lock files and fix test * Update packages/commerce-sdk-react/src/hooks/ShopperBaskets/index.test.ts * Rebuild lockfiles using node 14 npm 8 * Revert "Rebuild lockfiles using node 14 npm 8" This reverts commit 3d5c0cb. * Use pwa-kit-dev for lint and format * Revert "Use pwa-kit-dev for lint and format" This reverts commit f46d83e. * Add typescript to internal-lib-build and rebuild lock files --------- Co-authored-by: vcua-mobify <[email protected]> Co-authored-by: vcua-mobify <[email protected]> * [V2] Internal lib build typescript dev dependency (#1194) * Move typescript to dev and peer dependency * Update package lock file * Lockfile updates --------- Co-authored-by: Ben Chypak <[email protected]> Co-authored-by: Adam Raya <[email protected]> Co-authored-by: Vincent Marta <[email protected]> Co-authored-by: Oliver Brook <[email protected]> Co-authored-by: echessman <[email protected]> Co-authored-by: John Boxall <[email protected]> Co-authored-by: Richard Sexton <[email protected]> Co-authored-by: Will Harney <[email protected]> Co-authored-by: Kevin He <[email protected]> Co-authored-by: Alex Vuong <[email protected]> Co-authored-by: Brad Adams <[email protected]> Co-authored-by: Charles Lavery <[email protected]> Co-authored-by: Adam Raya Navarro <[email protected]> Co-authored-by: ecRobertEngel <[email protected]> Co-authored-by: Sandra Golden <[email protected]> Co-authored-by: Jainam Sheth <[email protected]> Co-authored-by: mdenchev-aiopsgroup <[email protected]>
Description
The initial simple solution is to keep using Chakra UI Link component in the PD ImageWithText Link component.
A more elaborated solution uses
isAbsoluteURL
util to conditionally use Chakra UI Link component for absolute URLs and our custom react-router Link component for relative URLs.TODO:
Types of Changes
Changes
How to Test-Drive This PR
template-retail-react-app
and load http://localhost:3000/page-viewer/get-ready-for-summer.Checklists
General
Accessibility Compliance
You must check off all items in one of the follow two lists:
or...
Localization